cmake: modules: Update zephyr_code_relocate API to support relocating libraries #50791

danieldegrasse · 2022-09-28T23:07:43Z

~~Add zephyr_library_relocate, a helper macro intended to allow entire Zephyr libraries to be relocated to a new memory region.~~

Update zephyr_code_relocate macro API to support the following:

A comma separated list of files (may be used with CMake generator expressions)
A CMake list of files (useful for file(GLOB ...)) directives
relocating all sources of CMake targets, such as all sources in a Zephyr library

Signed-off-by: Daniel DeGrasse [email protected]

carlocaione · 2022-10-03T12:18:39Z

Please update docs and samples

tejlmand

Please consider if the existing function can be extended to support generator expressions instead.

tejlmand · 2022-10-03T13:25:46Z

cmake/modules/extensions.cmake

  endforeach()
 endfunction(zephyr_linker_sources)

+function(zephyr_library_relocate location)


instead of creating a new function for this, which needs additional processing in the toplevel CMakeLists.txt file, I would much prefer extending the existing zephyr_code_relocate() function

zephyr/cmake/modules/extensions.cmake

Line 1302 in 04684c2

function(zephyr_code_relocate file location)

to accept generator expressions.

That avoids having one more function to maintain, and keeps other code clean.

Users can then do something like:

zephyr_code_relocate($<TARGET_PROPERTY:library,SOURCES> location)

A genex is not configure time processing dependent, making the code itself more robust (generally speaking).

As a small benefit, it will even allow downstream users to relocate files in a Zephyr library without patching Zephyr itself.

I've been looking into this, and I'm not sure how we can generically support CMake generator expressions in this function. If there's a CMake feature I'm unaware of you think might enable this, please let me know.

The core issue is that CMake generators don't seem to have a method of iterating over all files in the target's source property, so we can't handle relative files without changes to the python script. Even if these changes are made, I still don't know of a method to convert the file names to absolute paths using genex expressions, so the benefit you mentioned of downstream users being able to relocate a Zephyr library won't exist.

Another potentially more robust option would be to use the TARGET_OBJECTS property directly, since this is what the gen_relocate_app.py script will do internally. This would require API changes for the zephyr_code_relocate macro though, which I'd prefer to avoid.

The core issue is that CMake generators don't seem to have a method of iterating over all files in the target's source property, so we can't handle relative files without changes to the python script. Even if these changes are made, I still don't know of a method to convert the file names to absolute paths using genex expressions, so the benefit you mentioned of downstream users being able to relocate a Zephyr library won't exist.

Easy.
Add this code to any sample CMakeLists.txt and try to run ninja kernel_sources:

add_custom_target(kernel_sources COMMAND ${CMAKE_COMMAND} -E echo $<TARGET_PROPERTY:kernel,SOURCE_DIR>/$<JOIN:$<TARGET_PROPERTY:kernel,SOURCES>,$<COMMA>$<TARGET_PROPERTY:kernel,SOURCE_DIR>/> )

That will print:

/projects/github/ncs/zephyr/kernel/main_weak.c,/projects/github/ncs/zephyr/kernel/banner.c,/projects/github/ncs/zephyr/kernel/device.c,/projects/github/ncs/zephyr/kernel/errno.c,/projects/github/ncs/zephyr/kernel/fatal.c,/projects/github/ncs/zephyr/kernel/init.c,/projects/github/ncs/zephyr/kernel/kheap.c,/projects/github/ncs/zephyr/kernel/mem_slab.c,/projects/github/ncs/zephyr/kernel/thread.c,/projects/github/ncs/zephyr/kernel/version.c,/projects/github/ncs/zephyr/kernel/idle.c,/projects/github/ncs/zephyr/kernel/mailbox.c,/projects/github/ncs/zephyr/kernel/msg_q.c,/projects/github/ncs/zephyr/kernel/mutex.c,/projects/github/ncs/zephyr/kernel/queue.c,/projects/github/ncs/zephyr/kernel/sem.c,/projects/github/ncs/zephyr/kernel/stack.c,/projects/github/ncs/zephyr/kernel/system_work_q.c,/projects/github/ncs/zephyr/kernel/work.c,/projects/github/ncs/zephyr/kernel/sched.c,/projects/github/ncs/zephyr/kernel/condvar.c,/projects/github/ncs/zephyr/kernel/xip.c,/projects/github/ncs/zephyr/kernel/timeout.c,/projects/github/ncs/zephyr/kernel/timer.c,/projects/github/ncs/zephyr/kernel/poll.c,/projects/github/ncs/zephyr/kernel/mempool.c

Absolute path to all C files included in the kernel library, separated by a comma.

We probably want to provide an easy to use API for the users for the repeated pattern of including a library.
But if we support any generator expression, then we can easily create an easy to use flag.

For example:

zephyr_code_relocate(FILES <list of files or generator expression> LOCATION <location>)

but even allow a easy to use argument to the specific library case, which then will do a complex generator expression behind the scenes:

zephyr_code_relocate(LIBRARY <lib> LOCATION <location>)

Another potentially more robust option would be to use the TARGET_OBJECTS property directly

With generic genex support, anything is possible.
The user of the function decide which expressions to pass to the function.

This would require API changes for the zephyr_code_relocate macro though, which I'd prefer to avoid.

Fair point, but considering this PR and #49454, then I believe it's time to cleanup the API for this function and ensure it can handle more use-cases.

@danieldegrasse if you're not in a hurry for this, i will be happy to take a look at improving the function myself when having the time, but you are of course more than welcome to play around with this yourself.

I've updated the zephyr_code_relocate() macro based on your suggestions above. Thanks for the pointers- my knowledge on CMake is a bit lacking (especially regarding generator expressions).

tejlmand

Approved.
A minor comment, but nothing which should prevent merging as it can be updated in followup if need be.

tejlmand · 2023-01-12T13:35:35Z

cmake/modules/extensions.cmake

    set(copy_flag NOCOPY)
  endif()
+  if(CODE_REL_PHDR)
+    set(CODE_REL_LOCATION "${CODE_REL_LOCATION}\ :${CODE_REL_PHDR}")


exactly why is an escaped space needed here ?
That doesn't seem to have been necessary before.

The PHDR directive was previously paired with the relocation location, as you can see in this diff: https://github.com/zephyrproject-rtos/zephyr/pull/50791/files#diff-7087c4d7b0a6595b501520d5eb10ed5ecb01b3540831e3a141a6a662632ac940R20. This directive makes sure that the PHDR is still paired with the code location, so it can be parsed by gen_relocate_app.py. gen_relocate_app.py treats the PHDR as being paired with the relocation location, so this line preserves that behavior with the new API. An alternative here would be to integrate PHDR as a parameter like NOCOPY is used.

…tors Update gen_relocate_app.py to use "|" to separate code relocation directives, and ";" to separate multiple files in a relocation directive. This will enable multiple files to be passed to zephyr_code_relocate, as well as multiple files to be passed from a CMake generator expression. The script will then seperate these files and relocate each according to the arguments given to zephyr_code_relocate. Note! This commit will break support for zephyr_code_relocate until the CMake function is updated Signed-off-by: Daniel DeGrasse <[email protected]>

…aries Update API for zephyr_code_relocate to support cmake generator expressions, as well as relocating libraries. zephyr_code_relocate can now accept a target name to the LIBRARY argument, which will be converted into a set of source files from that target to relocate. Alternatively, files can be passed as a space separated list or CMake generator expression. This allows users more flexibility when relocating files. Glob matching functionality is still available, although the preferred method to do this would now be: file(GLOB relocate_sources "src/*.c") zephyr_code_relocate(FILES ${relocate_sources} LOCATION <location>) Note! This commit breaks support for zephyr_code_relocate until in tree usages of the API are updated to the new format. Signed-off-by: Daniel DeGrasse <[email protected]>

Update usage of zephyr_code_relocate to follow new API. The old method of relocating a file was the following directive: zephyr_code_relocate(file location) The new API for zephyr_code_relocate uses the following directive: zephyr_code_relocate(FILES file LOCATION location) update in tree usage to follow this model. Also, update the NXP HAL SHA, as NXP's HAL uses this macro as well. Signed-off-by: Daniel DeGrasse <[email protected]>

Update usage of zephyr_code_relocate to new API, and add examples of relocating a library target, as well as using multiple files in list or CMake generator expressions. Signed-off-by: Daniel DeGrasse <[email protected]>

Add test cases for generator expressions and library relocation to the code_relocation test, in order to verify functionality for these new API features. Signed-off-by: Daniel DeGrasse <[email protected]>

danieldegrasse · 2023-01-17T15:15:58Z

@tejlmand PR is updated and passing CI

danieldegrasse requested review from nashif and tejlmand as code owners September 28, 2022 23:07

danieldegrasse mentioned this pull request Sep 28, 2022

[DNM] net: allow relocation of network subsystem to RAM #50792

Closed

zephyrbot added the area: Build System label Sep 28, 2022

zephyrbot requested a review from jeremybettis September 28, 2022 23:08

zephyrbot assigned tejlmand Sep 28, 2022

danieldegrasse mentioned this pull request Sep 30, 2022

build: set recursive option for globbing relocated files #49454

Closed

carlescufi requested review from carlocaione, dcpleung and stephanosio October 3, 2022 12:11

tejlmand requested changes Oct 3, 2022

View reviewed changes

danieldegrasse requested review from andyross, dleach02, gmarull, mmahadevan108 and nvlsianpu as code owners October 4, 2022 23:49

danieldegrasse changed the title ~~cmake: modules: add zephyr_library_relocate, to relocate zephyr library~~ cmake: modules: Update zephyr_code_relocate API to support relocating libraries Oct 4, 2022

zephyrbot added area: Kernel area: Flash platform: NXP NXP area: Userspace Userspace labels Oct 4, 2022

zephyrbot requested review from DerekSnell, EmilioCBen, ceolin, decsny, enjiamai and peter-mitsis and removed request for dcpleung and jeremybettis October 4, 2022 23:50

carlescufi requested review from JordanYates and removed request for EmilioCBen, decsny, edersondisouza, enjiamai, gmarull, mmahadevan108 and peter-mitsis December 21, 2022 10:29

tejlmand previously approved these changes Jan 12, 2023

View reviewed changes

danieldegrasse added 2 commits January 16, 2023 17:32

danieldegrasse dismissed tejlmand’s stale review via 45e7958 January 16, 2023 23:56

zephyrbot requested review from jeremybettis, mmahadevan108, nordicjm and yvanderv January 16, 2023 23:56

zephyrbot added area: Documentation Release Notes To be mentioned in the release notes labels Jan 17, 2023

danieldegrasse added 3 commits January 17, 2023 08:25

doc: code-relocation: Update usage of zephyr_code_relocate

48aecc2

Update usage of zephyr_code_relocate to new API, and add examples of relocating a library target, as well as using multiple files in list or CMake generator expressions. Signed-off-by: Daniel DeGrasse <[email protected]>

tests: code_relocation: Add additional test cases for library and genex

88c5c87

Add test cases for generator expressions and library relocation to the code_relocation test, in order to verify functionality for these new API features. Signed-off-by: Daniel DeGrasse <[email protected]>

zephyrbot removed the DNM This PR should not be merged (Do Not Merge) label Jan 17, 2023

carlescufi requested review from tari and tejlmand January 17, 2023 17:05

tejlmand approved these changes Jan 17, 2023

View reviewed changes

carlescufi approved these changes Jan 17, 2023

View reviewed changes

carlescufi merged commit bd593bf into zephyrproject-rtos:main Jan 17, 2023

danieldegrasse deleted the feature/zephyr_library_relocate branch January 17, 2023 17:10

57300 mentioned this pull request Jan 13, 2024

code_relocation: Add NOKEEP option #67506

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

cmake: modules: Update zephyr_code_relocate API to support relocating libraries #50791

cmake: modules: Update zephyr_code_relocate API to support relocating libraries #50791

Uh oh!

danieldegrasse commented Sep 28, 2022 •

edited

Loading

Uh oh!

carlocaione commented Oct 3, 2022

Uh oh!

tejlmand left a comment

Uh oh!

tejlmand Oct 3, 2022

Uh oh!

danieldegrasse Oct 3, 2022

Uh oh!

tejlmand Oct 4, 2022

Uh oh!

tejlmand Oct 4, 2022

Uh oh!

danieldegrasse Oct 4, 2022

Uh oh!

tejlmand left a comment

Uh oh!

tejlmand Jan 12, 2023

Uh oh!

danieldegrasse Jan 17, 2023

Uh oh!

danieldegrasse commented Jan 17, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

cmake: modules: Update zephyr_code_relocate API to support relocating libraries #50791

cmake: modules: Update zephyr_code_relocate API to support relocating libraries #50791

Uh oh!

Conversation

danieldegrasse commented Sep 28, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

carlocaione commented Oct 3, 2022

Uh oh!

tejlmand left a comment

Choose a reason for hiding this comment

Uh oh!

tejlmand Oct 3, 2022

Choose a reason for hiding this comment

Uh oh!

danieldegrasse Oct 3, 2022

Choose a reason for hiding this comment

Uh oh!

tejlmand Oct 4, 2022

Choose a reason for hiding this comment

Uh oh!

tejlmand Oct 4, 2022

Choose a reason for hiding this comment

Uh oh!

danieldegrasse Oct 4, 2022

Choose a reason for hiding this comment

Uh oh!

tejlmand left a comment

Choose a reason for hiding this comment

Uh oh!

tejlmand Jan 12, 2023

Choose a reason for hiding this comment

Uh oh!

danieldegrasse Jan 17, 2023

Choose a reason for hiding this comment

Uh oh!

danieldegrasse commented Jan 17, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

danieldegrasse commented Sep 28, 2022 •

edited

Loading